Skip to content

fix(gnovm): correct softfloat add/sub for normal operands cancelling to subnormal#5818

Open
omarsy wants to merge 1 commit into
gnolang:masterfrom
omarsy:claude/magical-haslett-4f3d29
Open

fix(gnovm): correct softfloat add/sub for normal operands cancelling to subnormal#5818
omarsy wants to merge 1 commit into
gnolang:masterfrom
omarsy:claude/magical-haslett-4f3d29

Conversation

@omarsy

@omarsy omarsy commented Jun 11, 2026

Copy link
Copy Markdown
Member

Description

Fixes #5806.

gno run and go run disagree on float64 +/- for certain operands: when two near-equal, opposite-sign normal float64 values cancel into a subnormal result (|x+y| < 2.2e-308), gno returns a wrongly-scaled value (off by ~6 orders of magnitude), e.g. the issue's MRE returns 202154086 instead of 847895691526144.

Root cause

gno's softfloat (runtime_softfloat64.go) is a verbatim copy of Go's runtime/softfloat64.go, and the bug is in upstream's fpack64. Under heavy cancellation, fadd64 hands fpack64 a mantissa far below 1<<mantbits64 while the exponent is still normal-range. The denormal branch resets to the un-normalized (mant0, exp0) and only right-shifts to align to the subnormal exponent — the wrong direction in this case — so it returns the un-normalized cancellation mantissa at the wrong scale instead of the correctly-rounded subnormal.

This is latent in upstream Go too (it only executes on software-float targets like GOMIPS=softfloat); gno hits it because it always uses softfloat for deterministic, hardware-independent results.

Fix

Re-normalize the mantissa (left-shift) before the subnormal alignment loop — restoring the normalization invariant fpack64 already applies on its normal path (the identical loop runs at the top of the function). It is a no-op for already-normalized callers (fmul64/fdiv64/conversions), so they are unaffected; only the fadd64/fsub64 cancellation case changes. fpack32 carries the identical latent bug and is fixed for parity.

Because the file is generated (DO NOT EDIT, copied from $GOROOT), the fix is applied through the generator so it survives go generate, with an anchor guard that fails loudly if a future Go toolchain changes or fixes the upstream code.

Changes

  • Bug fixgnovm/pkg/gnolang/internal/softfloat/runtime_softfloat64.go: left-normalization loop added to the denormal branch of fpack64 and fpack32.
  • Toolinggnovm/pkg/gnolang/internal/softfloat/gen/main.go: applies the fix during go generate (with a strings.Count==1 anchor guard) plus injects the regression case into the generated test.
  • Testgnovm/pkg/gnolang/internal/softfloat/runtime_softfloat64_test.go: two cancellation operands added to TestFloat64's all-pairs (soft-vs-hardware) base slice.

Verification

  • The issue's add/sub operands now return 847895691526144, matching go run.
  • 0 mismatches vs hardware across 40M cancellation pairs (16M subnormal) and 20M random pairs over + - * /; the previous code was wrong on >50% of cancellation-to-subnormal pairs.
  • The reachable fpack32 path (f64->f32 narrowing) and full conversion sweeps match hardware; the new loop is a verified no-op for non-add/sub callers.
  • The same fix corrects a real GOMIPS=softfloat go1.22.3 build under qemu-mips (202154086 -> 847895691526144).

This bug has also been reported upstream to Go; once fixed there, the generator's anchor guard will trip on the next toolchain bump so the local workaround can be removed.

@github-actions github-actions Bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jun 11, 2026
@Gno2D2 Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Jun 11, 2026
@Gno2D2

Gno2D2 commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: omarsy/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🟢 User davd-gzl already reviewed PR 5818 with state APPROVED
    │       ├── 🔴 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

…to subnormal

gno's softfloat (a verbatim copy of Go's runtime/softfloat64.go) returns a
wrongly-scaled subnormal when two near-equal, opposite-sign normal float64
values cancel into a subnormal result (|x+y| < 2.2e-308). fadd64/fsub64 hand
fpack64 a heavily-cancelled mantissa (mant0 << 1<<mantbits64) while exp0 is
still a normal-range exponent; the denormal branch resets to (mant0, exp0) and
only right-shifts, which is the wrong direction in that case.

Fix: re-normalize the mantissa (left-shift) before aligning to the subnormal
exponent. This is a no-op for already-normalized callers (mul/div/conversions),
so it cannot regress them. The fix is applied through the generator so it
survives `go generate`, with an anchor guard that fails loudly if upstream Go
changes or fixes the code. fpack32 carries the identical latent bug and is
patched for parity.

Verified: the reported add/sub operands now return 847895691526144 (matching
`go run`); 0 mismatches vs hardware across 30M cancellation pairs (17M
subnormal) and 20M random pairs over +,-,*,/; the same fix corrects a real
GOMIPS=softfloat go1.22.3 build under qemu-mips. A regression case is added to
the softfloat64_test.go all-pairs base slice.

Fixes gnolang#5806

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@omarsy omarsy force-pushed the claude/magical-haslett-4f3d29 branch from e554fa0 to cd7b76c Compare June 11, 2026 19:15

@davd-gzl davd-gzl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Verified on cd7b76c: the issue MRE returns 847895691526144 for both add and sub and fails again with the fix reverted, a 30M+ pair sweep shows 0 mismatches vs hardware, and the generator reproduces both committed files byte-for-byte under a different Go toolchain.

Full review: https://github.com/samouraiworld/gno-agent-workspace/blob/main/reviews/pr/5xxx/5818-softfloat-subnormal-cancellation/1-cd7b76ca9/review_claude-opus-4-8_davd-gzl.md

(AI Agent)

// We insert a left-normalization loop before the subnormal alignment.
//
// See https://github.com/gnolang/gno/issues/5806. This has also been reported
// upstream; once Go fixes it, this patch's anchors will no longer be found and

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add link to the Go issue if existing

Comment on lines +52 to +53
math.Float64frombits(9333378022939403091),
math.Float64frombits(110005986185704326),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These regression operands are bare Float64frombits literals, unlike the neighbours that carry their value in a comment (// first normal). Appending the decimals (-2.662e-301 / 2.662e-301) makes the test data self-documenting.

(AI Agent)

@Gno2D2 Gno2D2 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📦 🤖 gnovm Issues or PRs gnovm related

Projects

Development

Successfully merging this pull request may close these issues.

float64 addition/subtraction returns wrong results for some operands

3 participants